-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: app wiring simulation failures #12051
Conversation
@@ -416,6 +416,7 @@ func NewSimApp( | |||
staking.NewAppModule(appCodec, app.StakingKeeper, app.AccountKeeper, app.BankKeeper), | |||
distr.NewAppModule(appCodec, app.DistrKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper), | |||
slashing.NewAppModule(appCodec, app.SlashingKeeper, app.AccountKeeper, app.BankKeeper, app.StakingKeeper), | |||
params.NewAppModule(app.ParamsKeeper), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The (re)addition of this line resolves
Simulating... block 5/50, operation 500/753. Logs to writing to /home/mkoco/.simapp/simulations/2022-05-25_16:39:13.log
simulate.go:302: error on block 5/50, operation (534/753) from x/staking:
failed to execute message; message index: 0: amount is greater than the unbonding delegation entry balance: invalid request [/home/mkoco/go/pkg/mod/cosmossdk.io/errors@v1.0.0-beta.6/errors.go:187]
Comment: unable to deliver tx
But I don't understand how or why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is correct actually. The runtime app doesn't have a simulation manager yet. But we should add that
0d0e129
to
d94cee3
Compare
b8222a2
to
eeb4a10
Compare
@@ -544,7 +545,17 @@ func (app *SimApp) InterfaceRegistry() codectypes.InterfaceRegistry { | |||
// | |||
// NOTE: This is solely to be used for testing purposes. | |||
func (app *SimApp) GetKey(storeKey string) *storetypes.KVStoreKey { | |||
return app.keys[storeKey] | |||
kvsk := app.keys[storeKey] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must do this for MemKeys and TransientKeys too (found out while working on x/capability)
fb49ca1
to
d90d8d9
Compare
* source: @alexanderbez #12052
Co-authored-by: Aaron Craelius <aaron@regen.network>
Codecov Report
@@ Coverage Diff @@
## main #12051 +/- ##
==========================================
- Coverage 66.09% 66.09% -0.01%
==========================================
Files 671 671
Lines 70917 70927 +10
==========================================
+ Hits 46874 46876 +2
- Misses 21385 21394 +9
+ Partials 2658 2657 -1
|
d90d8d9
to
1993be8
Compare
Description
Resolves simulation test failures introduced in #11924.
x/staking failure
Symptom of this failure is the descriptive log message
originating in x/staking/keeper/msg_server.CancelUnbondingDelegation. I was not able to find a concrete fix other than re-adding a new instance of the
params
module back toSimApp
's call tomodule.NewSimulationManager
which evidently has a side-effect I do not understand.store test failure
error log:
originating in sim_test.go. The root cause here seems to be test code which either fetches from
SimApp.keys
directly or usesSimApp.GetKey
to fetch aStoreKey
instance in order to build a keeper for various testing functions. Use of this pattern is ubiquitous:To this end a store key fetching function was added to
runtime/app
. As more keepers are moved over to the runtime they should just work in test asSimApp.GetKey
now falls back toApp.FindStoreKey
on not found. I am not a huge fan of this approach since it's exposingruntime
internals, alternative approaches include:runtime
inSimApp
FindStoreKey
by deafultAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change